-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SOF-7096: Typescript #92
Conversation
package.json
Outdated
"eslint-plugin-simple-import-sort": "^7.0.0", | ||
"eslint-plugin-react": "7.30.0", | ||
"eslint-plugin-simple-import-sort": "7.0.0", | ||
"eslint-import-resolver-exports": "^1.0.0-beta.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we replace these with the eslint plugin from npm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am afraid we need all of this. From README
1. Install config with eslint and eslint plugins
npm install --save-dev \
@exabyte-io/eslint-config \
@babel/[email protected] \
@babel/[email protected] \
@babel/[email protected] \
@babel/[email protected] \
@babel/register@^7.16.0 \
@babel/[email protected] \
@typescript-eslint/[email protected] \
@typescript-eslint/[email protected] \
[email protected] \
[email protected] \
[email protected] \
eslint-import-resolver-exports@^1.0.0-beta.2 \
eslint-import-resolver-meteor@^0.4.0 \
eslint-import-resolver-node@^0.3.6 \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
eslint-plugin-mui-path-imports
src/lattice/lattice_bravais.ts
Outdated
import { LATTICE_TYPE_CONFIGS } from "./types"; | ||
import { LATTICE_TYPE_CONFIGS, LatticeType, Vector, VectorsAsArray } from "./types"; | ||
|
||
export interface BravaisConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to get this and the below from ESSE schema, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Replaced this with LatticeImplicitSchema
src/lattice/lattice_vectors.ts
Outdated
import { BravaisConfig } from "./lattice_bravais"; | ||
import { LatticeType, Vector } from "./types"; | ||
|
||
interface LatticeVectorsConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with LatticeExplicitUnit
src/lattice/types.ts
Outdated
TRI_2a: "TRI_2a", | ||
TRI_1b: "TRI_1b", | ||
}; | ||
export enum LatticeType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 16 no longer relevant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/lattice/types.ts
Outdated
RHL = "RHL", | ||
MCL = "MCL", | ||
MCLC = "MCLC", | ||
TRI = "TRI", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any way we could move these enums to ESSE also, to be able to potentially reuse in Python in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have defined LatticeType
in ESSE LatticeImplicitSchema
, so we can use them from there.
But I didn't find LatticeTypeExtended
there. We can define another schema for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: we will be using strings instead of enums
bcherny/json-schema-to-typescript#200
src/types.ts
Outdated
import { ConstrainedBasisJSON } from "./basis/constrained_basis"; | ||
import { LatticeJSON } from "./lattice/lattice"; | ||
|
||
export interface MaterialJSON extends AnyObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
src/cell/primitive_cell.ts
Outdated
(x) => math.unit(x, "degree").to("rad").value, | ||
); | ||
|
||
const [cosAlpha, cosBeta, cosGamma] = [alpha, beta, gamma].map((x) => math.cos(x)); | ||
const [sinAlpha, sinBeta, sinGamma] = [alpha, beta, gamma].map((x) => math.sin(x)); | ||
const [sinAlpha, sinBeta] = [alpha, beta, gamma].map((x) => math.sin(x)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove gamma
from the right side as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/material.ts
Outdated
return this.toJSONDefault(); | ||
interface MaterialSchemaJSON extends MaterialSchema, AnyObject {} | ||
|
||
interface CheckResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could take this from esse as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/material.ts
Outdated
super(config); | ||
this._json = lodash.cloneDeep(config || {}); | ||
} | ||
interface Property { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be from ESSE too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DerivedPropertiesSchema doesn't have name
and value
properties but we use them in getDerivedPropertyByName()
and getInchiStringForHash()
.
Should I add it to to esse schema?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
No description provided.